Convention is: If the signature is exactly 64 bytes, it's an implied SigHashDefault. If it's 65 bytes, it is a different sighash and the 65th byte is the sighash byte.
We probably also need some logic to find out which entry of the witness is an actual signature.

Hmm... and that just get me thinking. Maybe we need a different way of transporting the sighash flag than by appending it to the witness. Because we look at the sighash on the level "vPSBT -> dummy TX". But the underlying Taproot execution VM will look at the sighash on the level "dummy TX -> txsighash", where we'll probably want to always use SigHashDefault. So the actual signature shouldn't contain the sighash as on the lowest level we always want it to be SigHashDefault. But when validating the asset, we need to know what the "vPSBT -> dummy TX" sighash was. So we need to transport it somewhere.

Or maybe this is an argument for actually mapping the vPSBT inputs/outputs directly into dummy TX inputs/outputs, then we can use the same sighash logic on both levels.

Right, thanks for writing this up, seems like there's loss of information by using the 1-1 dummy tx if we want to use more sighash flags.

One thing that I hadn't realized is that when the vPSBT is no longer around, a future verifier will have to rely on the ChallengeWitness field of Proof in order to check that a past transfer was valid. That ChallengeWitness currently is the single 1-1 dummy tx (which relied on tap always using "sighash all"). This means that the ephemeral dummy txs constructed by each vInput in order for the backend to sign will no longer be reconstructible by the verifier in order to check the signature.

As you mention, we can expand the ChallengeWitness and have multiple inputs and outputs. I believe we can align the inputs and outputs so that at input[i] / output[i] you'll find the roots of the trees into which the inputs and outputs were inserted, with respect to that vInput's sighash flag. So if on the vPSBT level we have 10 vInputs and 10 vOutputs, if the first (index 0) vInput wants to use a sighash that commits to a single input and all the outputs then on the dummy tx level we'd have input[0] be produced by inserting to an ms-smt just vInput[0] and output[0] would be produced by inserting all vOutputs in an ms-smt.

Can we go even further than that and do a "raw" mapping?

We can have each input in the dummy tx be produced by inserting into an ms-smt just the one vInput that has the same index. We can do exactly the same for the outputs, with each one being produced by an ms-smt that only contains the single vOutput on the same index. At that point can we just leverage the sighashes on the btc level? Nobody knows that the inputs/outputs are dummy, but the signatures can normally apply to them in any sighash fashion already available to us via the backend.

If the latter works I believe it's the preferable approach?
LMK what you think

I don't think this is an issue and is very likely just a misunderstanding around the ChallengeWitness.
That field is a special field that is only set in ownership proof files. An ownership proof is only created if you want/need to prove to someone that you do have the asset-level key to spend an asset. So it is just a signature showing you could spend the asset, without actually spending it (so it's not a valid transfer proof).

So the field is only set for the output of ProveAssetOwnership and can only be verified with VerifyAssetOwnership.
The 1-in-1 out mentioned there is actually on the vPSBT level (not the dummy TX level), so it won't be affected by any changes we make due to the sighash implementation. Also, for the asset ownership proof we'll likely always use SigHashDefault.

We can have each input in the dummy tx be produced by inserting into an ms-smt just the one vInput that has the same index. We can do exactly the same for the outputs, with each one being produced by an ms-smt that only contains the single vOutput on the same index. At that point can we just leverage the sighashes on the btc level? Nobody knows that the inputs/outputs are dummy, but the signatures can normally apply to them in any sighash fashion already available to us via the backend.

Yes, I think that is what we want. We'd do a raw mapping and then just use the BTC-VM level sighash functionality.

Main motivation here is that vm.ValidateWitnessV0 relies on constructing the virtual transaction with single input / single output. We can't break that now

See above. We can't unconditionally make the new tree.

This can prove problematic for some sighash types. As this output implicitly commits to all outputs, altering the desired effect of the sighash

Yeah, you're right. I think we won't be able to support the SIGHASH_SINGLE use case on the asset level. Because when you create the outputs, you need to know all outputs, otherwise you can't create a split commitment.
But looking at the example given in #577 it seems like we mostly want the SIGHASH_NONE functionality (at least on the output side). So that should still be achievable.

Yeah, you're right. I think we won't be able to support the SIGHASH_SINGLE use case on the asset level.

Agreed. Though I think it doesn't matter, as the root bitcoin signature is what actually binds everything. See my comment above. We don't need to enumerate the outputs as the split does that for us already.

nit: rename to txIns to not shadow the name in the loop below (for _, txIn := range txIn)?

This looks a bit confusing, as if the inputs and outputs were the same... Maybe it would be a bit more clear if the first parameter would just be the []PreviousWitness slice, since it looks like only that is being used for the input part of the virtual TX?

Yeah why do we need to pass in the asset twice? I don't see why we need to make any changes here. I think also shows the rationale re the design for the splits: the root split includes the commitment to them all, so you only need to pass around the root split for validation purposes.

nit: could make use of the fn.Map() function here. Doesn't really make it shorter but makes review a bit easier IMO (since creation of the slice with make is abstracted away).

Yeah, you're right. I think we won't be able to support the SIGHASH_SINGLE use case on the asset level. Because when you create the outputs, you need to know all outputs, otherwise you can't create a split commitment.
But looking at the example given in #577 it seems like we mostly want the SIGHASH_NONE functionality (at least on the output side). So that should still be achievable.

So this can't be done unconditionally. We need to make a new TAP VM version, then flatten the virtual transaction only for that version.

FWIW, coming back to this, I think I'm still not totally convinced we need to do this. Input level conditional signing can happen on the Bitcoin layer, in that you use the normal sig hash flags there to bind your Bitcoin level signature to the presence of one or all inputs.

In TAP, we have a sort of output centric model: you validate the output (asset leaf TLV), and the witness data is also stored in the output. You can control the environment needed to validate that output based on the logical spend group. The set of inputs here can only ever be the set of inputs referenced in the witness data for the output. Each input needs a witness, and is effectively always consolidating to that single output (ignoring splits for a second).

With the framing above, in what case would we ever need to sign only some of the inputs in that spend group? Changing the set of inputs actually changes the output, so those need to be fully bound. We don't have the ability to have loose binding of the inputs.

If you consider splits, then that's an area that I think we can already have covered as the sighash flag can determine which of the split out puts are inserted into the split output tree. For all, do everything. For none, do nothing. You can make single work somewhat

With all that said, I think it makes sense to make this an option, if we have indeed found a flow that can't work w/o a flattened virtual txn. Why wouldn't we also flatten the tx out as well?

As mentioned above, this requires updates elsewhere.

So if we're not doing the 1-in-1-out version, then we don't need to swap out information for each input. Instead, the virtual tx has all the inputs, and idx here is just passed to sign/verify.

I don't think this needs to change. There's only ever a single virtual txin still.

Yeah why do we need to pass in the asset twice? I don't see why we need to make any changes here. I think also shows the rationale re the design for the splits: the root split includes the commitment to them all, so you only need to pass around the root split for validation purposes.

Yeah, you're right. I think we won't be able to support the SIGHASH_SINGLE use case on the asset level.

Agreed. Though I think it doesn't matter, as the root bitcoin signature is what actually binds everything. See my comment above. We don't need to enumerate the outputs as the split does that for us already.

Doesn't match the mock version in the tests above.

Yes, it's always first in the split commitment tree.

This should take the script version as input. We can't break all the old versions (but again, I don't think this level of changes are needed: output sighashes are all or nothing).

See above. We can't unconditionally make the new tree.